JS: provide command execution sinks for execa package #14294
Conversation
There was a problem hiding this comment.
Here is a first round of comments, it will take a while before we can get this ready to merge.
This is not an exhaustive review, but it should give you plenty to work on.
I suggest you only continue with the other library models after we've gotten this PR into a mergable state.
The tests in this PR are in general very confusing to read.
You're mixing file-system access with system-command execution, and nothing in the code/comments in tst.js tells me which is which.
You also have plenty of "GOOD" and "BAD" comments, but I can't figure out what they actually mean.
Just as an example: Line 7 and 9 of tst.js are annotated with "GOOD" and "BAD" respectively.
But they both work only on string constants, and are thus completely harmless, and the only thing you're testing is that they are both recognized as system-command-executions (which they should be).
Basically: there is no connection between the test outcomes (the .expected file) and the comments in the test.
Also, the tests are not testing very much.
E.g. your test_SystemCommandExecution is only testing that a command-execution is recognized, not what the command-argument or argument-list of that command-execution is.
Adding some parameters to the test predicates can help.
Another thing that would be really useful is to add some end-to-end tests.
E.g. adding a new JavaScript file in javascript/ql/test/query-tests/Security/CWE-078/CommandInjection/, where test specifically for command-execution, and annotate each line with // OK / // NOT OK similar to how other files in that directory do it.
A reason for second-order-command-injection not working is that you haven't implemented the optional getArgumentList predicate on the relevant class. When I add an implemtation of that predicate to ExecaExec I can get this example to work just fine: execa('git', ['ls-remote', unsafeValue]); // NOT OK.
|
@erik-krogh sorry for the many mistakes on this PR. I tried to solve all of your review suggestions. |
That's OK. We set a high bar here for testing and code quality, especially when you try to get something merged into our standard query set. In other news. I suggest you take a look at that PR and incorporate it into your PR after my PR has been merged. |
|
@erik-krogh it is great that I can reach each part of the following example with the API graph: $({shell: false}).sync`${cmd} ${arg} ${arg} ${arg2}`; // NOT OKI think there is one problem. $({shell: false}).sync`ssh ${arg} ${arg} ${arg2}`; // NOT OKthen I can't specify |
Well, you can, it's just a little tricky. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@erik-krogh |
That's up to you. |
|
@erik-krogh I moved this PR to experimental dir. |
Looks OK. I'll give this a final review when you mark this PR as ready for review. |
|
@erik-krogh this PR also is not marked as ready for review but you reviewed this before, can I mark this as Ready for review too? |
Yes 👍 |
|
The branch was getting old. I did a merge with Edit: Another merge with |
Execa package before version 5 has already been modeled but newer versions up to 8 have many new APIs that I've implemented now.
@erik-krogh the
SecondOrderCommandInjectionQuerydoesn't work for me here. please look at the test for examples, I already put comments on which lines are vulnerable to second-order command injection.